Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add support for specifying badEnclosingTypes for BadImport via flags #4228

Conversation

hisener
Copy link
Contributor

@hisener hisener commented Jan 1, 2024

Following the discussions in #2236, this PR introduces the BadImport:BadEnclosingTypes flag to disallow nested types of specified enclosing type.

This PR also takes the flag into account in UnnecessarilyFullyQualified to suggest importing EnclosingType.NestedType when possible.

For instance, when -XepOpt:BadImport:BadEnclosingTypes=org.immutables.value.Value is set, it would suggest the following:

final class Test {
  @org.immutables.value.Value.Immutable
  abstract class AbstractType {}
}

->

final class Test {
  @Value.Immutable
  abstract class AbstractType {}
}

Also, it won't require suppressing UnnecessarilyFullyQualified in the example in #2236.

import org.springframework.beans.factory.annotation.Value;

final class Test {
  Demo(@Value("some.property") String property) {}

  @org.immutables.value.Value.Immutable
  abstract class AbstractType {}
}

Closes #2236.

Comment on lines +434 to +446
@Test
public void badEnclosingTypes_staticMethod() {
compilationTestHelper
.setArgs("-XepOpt:BadImport:BadEnclosingTypes=com.google.common.collect.ImmutableList")
.addSourceLines(
"Test.java",
"import static com.google.common.collect.ImmutableList.toImmutableList;",
"import com.google.common.collect.ImmutableList;",
"import java.util.stream.Collector;",
"",
"class Test {",
" // BUG: Diagnostic contains: ImmutableList.toImmutableList()",
" Collector<?, ?, ImmutableList<Object>> immutableList = toImmutableList();",
Copy link
Contributor Author

@hisener hisener Jan 1, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is disallowing static import desirable? Should we change the behavior only for nested types? 🤔

if (!tree.isStatic()) {
symbol = getSymbol(tree.getQualifiedIdentifier());
if (symbol == null || isAcceptableImport(symbol, BAD_NESTED_CLASSES)) {

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The current behaviour of treating static imports consistently seems OK to me. Obviously toImmutableList is just an example, in general if com.example.Foo is supposed to provide a namespace and not have its members be imports, it seems OK to discourage both import com.example.Foo.Bar and import static com.example.Foo.baz.

@hisener hisener marked this pull request as ready for review January 1, 2024 10:25
Copy link
Collaborator

@cushon cushon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM aside from the comment about Tree#toString. Thanks for the PR!

@@ -138,7 +159,8 @@ private void handle(TreePath path) {
if (!isFullyQualified(tree)) {
return;
}
if (BadImport.BAD_NESTED_CLASSES.contains(tree.getIdentifier().toString())) {
if (exemptedTypes.contains(tree.getExpression().toString())
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is fine because of the guarantees made by the checks in isFullyQualified, but we generally try to avoid using Tree#toString. There's some performance cost for larger trees, and the string representation isn't stable.

What do you think about something like:

if (!BadImport.BAD_NESTED_CLASSES.contains(tree.getIdentifier().toString())) {
  return;
}
if (!(tree.getExpression() instanceof MemberSelectTree)) {
  return;
}
Symbol sym = getSymbol(tree.getExpression());
if (!(sym instanceof ClassSymbol)) {
  return;
}
if (exemptedTypes.contains(((ClassSymbol) sym).getQualifiedName().toString())) {
  return;
}
handle(new TreePath(path, tree.getExpression()));

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👀 We shouldn't directly return if it's not a bad nested class, but I think I got your point.

I tried to refactor in 67aa36d, PTAL.

Comment on lines +434 to +446
@Test
public void badEnclosingTypes_staticMethod() {
compilationTestHelper
.setArgs("-XepOpt:BadImport:BadEnclosingTypes=com.google.common.collect.ImmutableList")
.addSourceLines(
"Test.java",
"import static com.google.common.collect.ImmutableList.toImmutableList;",
"import com.google.common.collect.ImmutableList;",
"import java.util.stream.Collector;",
"",
"class Test {",
" // BUG: Diagnostic contains: ImmutableList.toImmutableList()",
" Collector<?, ?, ImmutableList<Object>> immutableList = toImmutableList();",
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The current behaviour of treating static imports consistently seems OK to me. Obviously toImmutableList is just an example, in general if com.example.Foo is supposed to provide a namespace and not have its members be imports, it seems OK to discourage both import com.example.Foo.Bar and import static com.example.Foo.baz.

return BadImport.BAD_NESTED_CLASSES.contains(tree.getIdentifier().toString());
}

private boolean isExemptedEnclosingType(MemberSelectTree tree) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does it make sense to early-exit if the flag is not set? Or, is the cost negligible? 🤔

Suggested change
private boolean isExemptedEnclosingType(MemberSelectTree tree) {
private boolean isExemptedEnclosingType(MemberSelectTree tree) {
if (exemptedEnclosingTypes.isEmpty()) {
return false;
}

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Either way is fine with me, I think the cost is probably negligible.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Leaving it as-is then 👍

copybara-service bot pushed a commit that referenced this pull request Jan 2, 2024
Following the discussions in #2236, this PR introduces the `BadImport:BadEnclosingTypes` flag to disallow nested types of specified enclosing type.

This PR also takes the flag into account in `UnnecessarilyFullyQualified` to suggest importing `EnclosingType.NestedType` when possible.

For instance, when `-XepOpt:BadImport:BadEnclosingTypes=org.immutables.value.Value` is set, it would suggest the following:

```java
final class Test {
  @org.immutables.value.Value.Immutable
  abstract class AbstractType {}
}
```
->
```java
final class Test {
  @Value.Immutable
  abstract class AbstractType {}
}
```

Also, it won't require suppressing `UnnecessarilyFullyQualified` in the example in #2236.

```java
import org.springframework.beans.factory.annotation.Value;

final class Test {
  Demo(@value("some.property") String property) {}

  @org.immutables.value.Value.Immutable
  abstract class AbstractType {}
}
```

Closes #2236.

Fixes #4228

FUTURE_COPYBARA_INTEGRATE_REVIEW=#4228 from hisener:halil.sener/bad-import-bad-enclosing-types 67aa36d
PiperOrigin-RevId: 595179563
copybara-service bot pushed a commit that referenced this pull request Jan 12, 2024
Following the discussions in #2236, this PR introduces the `BadImport:BadEnclosingTypes` flag to disallow nested types of specified enclosing type.

This PR also takes the flag into account in `UnnecessarilyFullyQualified` to suggest importing `EnclosingType.NestedType` when possible.

For instance, when `-XepOpt:BadImport:BadEnclosingTypes=org.immutables.value.Value` is set, it would suggest the following:

```java
final class Test {
  @org.immutables.value.Value.Immutable
  abstract class AbstractType {}
}
```
->
```java
final class Test {
  @Value.Immutable
  abstract class AbstractType {}
}
```

Also, it won't require suppressing `UnnecessarilyFullyQualified` in the example in #2236.

```java
import org.springframework.beans.factory.annotation.Value;

final class Test {
  Demo(@value("some.property") String property) {}

  @org.immutables.value.Value.Immutable
  abstract class AbstractType {}
}
```

Closes #2236.

Fixes #4228

FUTURE_COPYBARA_INTEGRATE_REVIEW=#4228 from hisener:halil.sener/bad-import-bad-enclosing-types 67aa36d
PiperOrigin-RevId: 595179563
@hisener hisener deleted the halil.sener/bad-import-bad-enclosing-types branch January 12, 2024 12:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants